I'd like to propose a set of default rules for browser/toolkit JS code. These are rules that block most code that is invalid or would be difficult to read plus some stylistic rules to keep things roughly consistent. This doesn't mean that all toolkit/browser code must follow it. Sub-directories can override these rules where it makes sense to follow existing style or because of other considerations. We would just consider these rules to be the defaults for new code unless otherwise changed. Initially this would probably be landed with all rules commented out until we have chance to update code to work.

Bug 1229097: Land an initial .eslintrc for browser and toolkit.

https://reviewboard.mozilla.org/r/26529/#review24021 just some comments ::: browser/.eslintrc:64 (Diff revision 1) > + "no-octal": 2, does this allow the ES6 octal format 0o600? (not the deprecated 0600) ::: browser/.eslintrc:111 (Diff revision 1) > + "array-bracket-spacing": [1, "never"], I disagree on this, spaces in array declarations are very useful for readability... ::: browser/.eslintrc:123 (Diff revision 1) > + "comma-style": 2, in some cases it was useful to define arrays as [ one , two , three ] to preserve blame with future additions/removals... ::: browser/.eslintrc:177 (Diff revision 1) > + "space-before-function-paren": [2, "never"], what we usually enforced in browser is {"anonymous": "always", "named": "never"} ::: browser/.eslintrc:183 (Diff revision 1) > + "space-in-parens": [2, "never"], will this allow things like somefunc([ "one", "two" ]) or somefunc({ one, two: 2 }) that are very useful for readability?

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 https://reviewboard.mozilla.org/r/26529/#review24025 r=me even if you don't take my comments. Not worth a bikeshed to get this through. Thanks for doing this! ::: browser/.eslintrc:27 (Diff revision 1) > + // No function declarations in inner blocks > + "no-inner-declarations": 2, AIUI this means you can't do things like: if (foo) { var bar = 5; } else { bar = 7; } and you are also not allowed inner functions: function foo() { function bar() { } // more code that sometimes calls bar(); } which will also disallow whole-file closures that hide things from external consumers. I think both of those are legitimately used in a lot of our codebase, and so I am not convinced erroring out for these is a good idea. ::: browser/.eslintrc:45 (Diff revision 1) > + // No unreachable statements > + "no-unreachable": 2, I wonder what the ratio for intentional/unintentional use is here. But let's try it with this and we'll see if it starts biting us. ::: browser/.eslintrc:86 (Diff revision 1) > + // No double semicolon > + "no-extra-semi": 2, I don't understand how this is different from "no empty statements", but there we go. ::: browser/.eslintrc:146 (Diff revision 1) > + // No single if block inside an else block > + "no-lonely-if": 1, I would nominate no-else-return here too, please. ::: browser/.eslintrc:149 (Diff revision 1) > + // No mixing spaces and tabs in indent > + "no-mixed-spaces-and-tabs": [2, "smart-tabs"], I am less sympathetic to tabs than you are, but I guess this will start fewer flamewars to start with.

https://reviewboard.mozilla.org/r/26529/#review24033 ::: browser/.eslintrc:164 (Diff revision 1) > + // Require padding inside object declarations > + "object-curly-spacing": [2, "always"], Actually, reviewing Marco's comments, I am confused about this being the inverse of the array one. Personally I write: var foo = [5, 3]; and var bar = {foo: "bar"}; I could live with using spaces everywhere, but I don't really understand why this is currently requiring spaces for objects and requiring no spaces for arrays. If this does turn into a bikeshedding thing, maybe let's focus on the functional stuff like undeclared variables and tighten up style at a later point?

https://reviewboard.mozilla.org/r/26529/#review24021 > I disagree on this, spaces in array declarations are very useful for readability... FWIW, as a compromise we could consider using the third option to allow it for nested objects/arrays, see the "exceptions" section on http://eslint.org/docs/rules/array-bracket-spacing .

(In reply to Marco Bonardo [::mak] from comment #2) > https://reviewboard.mozilla.org/r/26529/#review24021 > > just some comments > > ::: browser/.eslintrc:64 > (Diff revision 1) > > + "no-octal": 2, > > does this allow the ES6 octal format 0o600? (not the deprecated 0600) I believe so, if it doesn't we'll remove it. > ::: browser/.eslintrc:111 > (Diff revision 1) > > + "array-bracket-spacing": [1, "never"], > > I disagree on this, spaces in array declarations are very useful for > readability... To be clear, this allows |[2, 3, 4]| but disallows |[ 2, 3, 4 ]|. The former seems to be the norm that I've seen. > ::: browser/.eslintrc:123 > (Diff revision 1) > > + "comma-style": 2, > > in some cases it was useful to define arrays as > [ one > , two > , three > ] > to preserve blame with future additions/removals... I've seen use trailing commas to solve that in general: [ one, two, three, ] > ::: browser/.eslintrc:177 > (Diff revision 1) > > + "space-before-function-paren": [2, "never"], > > what we usually enforced in browser is > {"anonymous": "always", "named": "never"} That might make sense yes. > ::: browser/.eslintrc:183 > (Diff revision 1) > > + "space-in-parens": [2, "never"], > > will this allow things like somefunc([ "one", "two" ]) or somefunc({ one, > two: 2 }) that are very useful for readability? It only applies to spaces immediately after the opening parenthesis or before the closing one so those would work (depending on the array-bracket-spacing and object-curly-spacing settings). (In reply to :Gijs Kruitbosch from comment #3) > > + // No function declarations in inner blocks > > + "no-inner-declarations": 2, > > AIUI this means you can't do things like: Ah I missed that that applied to variables too. Mainly I included it because it is illegal in strict mode anyway, I can remove it for now though since when it fails in strict mode it will be a parse failure anyway. > (Diff revision 1) > > + // No unreachable statements > > + "no-unreachable": 2, > > I wonder what the ratio for intentional/unintentional use is here. But let's > try it with this and we'll see if it starts biting us. I'm assuming it is never needed, I can't think of an intentional case. > ::: browser/.eslintrc:86 > (Diff revision 1) > > + // No double semicolon > > + "no-extra-semi": 2, > > I don't understand how this is different from "no empty statements", but > there we go. > > ::: browser/.eslintrc:146 > (Diff revision 1) > > + // No single if block inside an else block > > + "no-lonely-if": 1, > > I would nominate no-else-return here too, please. I flip-flopped a little on that but lets see how it works out. > ::: browser/.eslintrc:149 > (Diff revision 1) > > + // No mixing spaces and tabs in indent > > + "no-mixed-spaces-and-tabs": [2, "smart-tabs"], > > I am less sympathetic to tabs than you are, but I guess this will start > fewer flamewars to start with. I believe the indent rule denies tab indentation. I put this here so inner-directories could allow tabs if they wanted but this rule would apply to at least keep thinks sane. (In reply to :Gijs Kruitbosch from comment #4) > https://reviewboard.mozilla.org/r/26529/#review24033 > > ::: browser/.eslintrc:164 > (Diff revision 1) > > + // Require padding inside object declarations > > + "object-curly-spacing": [2, "always"], > > Actually, reviewing Marco's comments, I am confused about this being the > inverse of the array one. Personally I write: > > var foo = [5, 3]; > > and > > var bar = {foo: "bar"}; > > I could live with using spaces everywhere, but I don't really understand why > this is currently requiring spaces for objects and requiring no spaces for > arrays. > > If this does turn into a bikeshedding thing, maybe let's focus on the > functional stuff like undeclared variables and tighten up style at a later > point? Yeah I think these two are going to be different enough between people that it probably doesn't make sense to include them. It's also a case where whether there is a space or not doesn't really make much difference to the code approachability IMO.

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26529/diff/1-2/

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 https://reviewboard.mozilla.org/r/26529/#review24039 r=me assuming the toolkit thing is just a duplicate of the browser one (can't tell easily from reviewboard).

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26529/diff/2-3/

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 https://reviewboard.mozilla.org/r/26529/#review24135 ::: browser/.eslintrc:108 (Diff revision 3) > + "no-unused-vars": [1, {"vars": "all", "args": "none"}] Missing trailing comma here.

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 No, mozreview, no.

(In reply to :Gijs Kruitbosch from comment #12) > Are we adding the es6 language options somewhere else? Because right now > this eslintrc doesn't seem to wfm as is because of that... Err, ignore me. I needed to update my tree. New issue though: switch (foo) { case 5: ..; } gets flagged for using the wrong indent (rule "indent"). So I think for indent we want: "indent": [2, 2, {"SwitchCase": 1}]

This also doesn't allow lining up key values: var obj = { foo: 3, longbar: 4, thingy: 5, }; because it complains about the excess space after the ":". It seems we could use {"beforeColon": false, "afterColon": true, "mode": "minimum"} to avoid it complaining about this (while retaining complaints about {foo : bar} or {foo:bar} ). That or maybe I'm wrong in thinking we use this style in places and/or we want to move away from this style?

(In reply to :Gijs Kruitbosch from comment #14) > This also doesn't allow lining up key values: > > var obj = { > foo: 3, > longbar: 4, > thingy: 5, > }; > > because it complains about the excess space after the ":". It seems we could > use > > {"beforeColon": false, "afterColon": true, "mode": "minimum"} > > to avoid it complaining about this (while retaining complaints about {foo : > bar} or {foo:bar} ). > > That or maybe I'm wrong in thinking we use this style in places and/or we > want to move away from this style? I think it makes sense to allow that.

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26529/diff/3-4/

wouldn't make more sense if browser would extend toolkit, rather than the opposite?

I think we should add "consistent" option to "curly", since one of the codinst style rules is that if one side of an if/else is braced, all should be. unfortunately sounds like curly multi-line allows: if (condition) statement; that is something we don't want per coding style, but disallows: if (condition) statement; that we use a lot (even if the coding style states we should always brace, but it was inherited from cpp and nobody ever cared that much)

Comment on attachment 8693704 [details] MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and toolkit. r=gijs, r?Standard8 https://reviewboard.mozilla.org/r/26529/#review24291 Looks reasonable in general to me - definitely some nice rules I want to look at enabling for Loop when I get chance :-) As Marco says we could slave this to toolkit, rather than toolkit to browser, or we could do something like create a .eslint-desktop at the top level and just extend toolkit and browser with that. I don't have a strong opinion on what we do there though. ::: browser/.eslintrc:7 (Diff revision 4) > + "no-dupe-args": 2, I would recommend putting these in alphabetical order within the sections - it makes it much easier to compare / find others that are enabled. ::: browser/.eslintrc:81 (Diff revision 4) > + "no-extra-boolean-cast": 1, Personally I'm not too keen on warnings as they don't force people to add them, and you get to a stage where you've got so many warnings you're not necessarily sure if more are being added or not. However, if the aim is to make these errors eventually, then I think its a reasonable intermediate step, but might be worth mentioning somewhere.

(In reply to Marco Bonardo [::mak] from comment #18) > I think we should add "consistent" option to "curly", since one of the > codinst style rules is that if one side of an if/else is braced, all should > be. > > unfortunately sounds like curly multi-line allows: > if (condition) statement; > that is something we don't want per coding style, but disallows: > if (condition) > statement; > that we use a lot (even if the coding style states we should always brace, > but it was inherited from cpp and nobody ever cared that much) I think curly is going to be a rule that we end up overriding on a per-directory basis very frequently since we have all sorts of different conventions throughout the tree (add-ons manager for instance tends towards [2, "multi", "consistent"]). To that end I've tried to put what matches the style guide as the default here but I'll add a comment suggesting overriding. These are all landing commented off for now anyway, it is likely we'll be tweaking each rule as we attempt to enable it. (In reply to Mark Banner (:standard8) from comment #19) > Comment on attachment 8693704 [details] > MozReview Request: Bug 1229097: Land an initial .eslintrc for browser and > toolkit. r=gijs, r?Standard8 > > https://reviewboard.mozilla.org/r/26529/#review24291 > > Looks reasonable in general to me - definitely some nice rules I want to > look at enabling for Loop when I get chance :-) > > As Marco says we could slave this to toolkit, rather than toolkit to > browser, or we could do something like create a .eslint-desktop at the top > level and just extend toolkit and browser with that. I don't have a strong > opinion on what we do there though. > > ::: browser/.eslintrc:7 > (Diff revision 4) > > + "no-dupe-args": 2, > > I would recommend putting these in alphabetical order within the sections - > it makes it much easier to compare / find others that are enabled. > > ::: browser/.eslintrc:81 > (Diff revision 4) > > + "no-extra-boolean-cast": 1, > > Personally I'm not too keen on warnings as they don't force people to add > them, and you get to a stage where you've got so many warnings you're not > necessarily sure if more are being added or not. > > However, if the aim is to make these errors eventually, then I think its a > reasonable intermediate step, but might be worth mentioning somewhere. Yeah I think I agree.

Seems like we've had enough discussion to go ahead and land this with the rules commented out. Next step will be working to turn them on. Some where eslint can do automated fixes will be pretty simple.

https://hg.mozilla.org/mozilla-central/rev/ed6fd38d7eb2 https://hg.mozilla.org/mozilla-central/rev/0ae21184dbe7